Skip to content

Filtering: NonApplicability improvements#1373

Open
mdvictor wants to merge 27 commits intomainfrom
mdv/non-applicable-filters-prometheus
Open

Filtering: NonApplicability improvements#1373
mdvictor wants to merge 27 commits intomainfrom
mdv/non-applicable-filters-prometheus

Conversation

@mdvictor
Copy link
Copy Markdown
Collaborator

@mdvictor mdvictor commented Feb 26, 2026

Improves the filtering non-applicability system:

  • cached pre-query filter and groupBy verification calls in the SQR
  • better applicability retrieval matching (e.g.: in the recommendations system), based on the fact that the DS will return the filters in the same order as they were sent
  • caching for global variable level non applicability
  • exposes getter to be used outside the drilldown manager, e.g.: in the panel subheader showing per panel non applicable filters and groupBys
📦 Published PR as canary version: 7.2.0--canary.1373.22950463934.0

✨ Test out this PR locally via:

npm install @grafana/scenes@7.2.0--canary.1373.22950463934.0
npm install @grafana/scenes-react@7.2.0--canary.1373.22950463934.0
# or 
yarn add @grafana/scenes@7.2.0--canary.1373.22950463934.0
yarn add @grafana/scenes-react@7.2.0--canary.1373.22950463934.0

@mdvictor mdvictor changed the title Mdv/non applicable filters prometheus Filtering: NonApplicability improvements Feb 26, 2026
const originFilters = this.state.originFilters?.filter((f) => f.key !== filter.key && !f.nonApplicable) ?? [];
const otherFilters = this.state.filters
.filter((f) => f.key !== filter.key && !f.nonApplicable)
.concat(originFilters);
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we need to check in values too for non applicability, otherwise if we have some nonapplicable filters and search for values, nothing will come up because of that nonexistent label being set on the query

@mdvictor mdvictor added minor Increment the minor version when merged release Create a release when this pr is merged labels Mar 3, 2026
const filtersApplicabilityEnabled = this._adhocFiltersVar?.state.applicabilityEnabled;
const groupByApplicabilityEnabled = this._groupByVar?.state.applicabilityEnabled;

if (!filtersApplicabilityEnabled && !groupByApplicabilityEnabled) {
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is basically the grafana FF check

@mdvictor mdvictor requested a review from dprokop March 5, 2026 09:27
@mdvictor mdvictor marked this pull request as ready for review March 9, 2026 12:21
Copy link
Copy Markdown
Collaborator

@dprokop dprokop left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea to centralize the logic. Please adress my comments before we continue.


this._drilldownDependenciesManager.findAndSubscribeToDrilldowns(ds.uid, this);

await this._drilldownDependenciesManager.resolveApplicability(
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to await here? This is gonna block query execution. Maybe instead just fire this funtion without await.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could let it fire and not block querying but then we would cancel inflights and requuery as soon as applicability resolves so we get the correct filters for the given queries

}
});

this.setState({ applicabilityEnabled: false });
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is ths intentional?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, that was causing some issues now that we set that prop through the ff in grafana

export function buildApplicabilityCacheKey(parts: {
filters?: Array<{ origin?: string; key: string; operator: string; value: string; values?: string[] }>;
groupByKeys?: string[];
value?: string[];
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's this?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we use this util to limit applicability calls -- if nothing changes, then we dont rerun the call with the same values basically

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh nvm I think you're asking about the specific value param, I think that's not needed and I missed it, will doublecheck


const stateFilters = this._adhocFiltersVar.state.filters;
const originFilters = this._adhocFiltersVar.state.originFilters ?? [];
const allFilters = [...originFilters, ...stateFilters];
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sanity check - does it matter that in the adhoc filters we create const filters = [...this.state.filters, ...(this.state.originFilters ?? [])]; ? Reversed order

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good call, yeah this should be consistent and originfilters should be first by convention

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've modified the adhoc to be consistent, the origin filters were put last mainly because it was easier and order didnt matter -- it still doesnt for origin ones vs non origin ones -- it matters mostly for same type of filters with same key (e.g. I have 2 user filters env=prod and then env=dev, the latter one should take priority and overwrite the first, this doesnt happen for origin and user ones because of the origin property delimitation)

Comment on lines +48 to +49
groupByKeys?: string[];
value?: string[];
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why did you separate these two? IIUC these properties are used in the code to represent the same thing - group by keys.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes I think you are right, the value is unneeded

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed value, unneeded

}

function normalizeQuery(q: SceneDataQuery): { refId: string; expr?: unknown } {
return { refId: q.refId, expr: q.expr ?? q.expression ?? q.query };
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very prometheus-yyyyyy :)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤦🏼 focused so much on prometheus this slipped, I'm looking into it

const json = store.get(this._getStorageKey());
const storedGroupings = json ? JSON.parse(json) : [];

if (storedGroupings.length > 0) {
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

got rid of these extra requests for now since this will also be unified

return applicableValues;
}

public async getGroupByApplicabilityForQueries(
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here, just removed functionality to have less requests, again will be solved by unification later

return this._resolvedDs;
}

const ds = await this._dataSourceSrv.get(this.state.datasource, this._scopedVars);
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reusing scopedVars from the adhoc in the recomendations as well ( through this method that is reused there), i reckon thats fine

it('should add value to parent variable', async () => {
const getRecommendedDrilldowns = jest.fn().mockResolvedValue(undefined);
const getDrilldownsApplicability = jest.fn().mockResolvedValue(undefined);
const getDrilldownsApplicability = jest.fn().mockResolvedValue(new Map([['_default_', []]]));
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

at this point i think everything groupBy can be ignored, this will be removed after unification

(f) => isFilterComplete(f) && isFilterApplicable(f)
)
: undefined;
if (!this._adhocFiltersVar) {
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this entire manager will get very thin after the unification, basically it would just deal with adhoc subscription and fetching filters... might not even need it at all we'll see

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

minor Increment the minor version when merged release Create a release when this pr is merged

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants